Conversation
- 기존 useModal 훅 기반의 선언적 구조에서 modalStore 기반의 명령형 구조로 전환 - observer Pattern을 활용하여 컴포넌트 내부 JSX와 모달 렌더링 로직의 결합도를 분리
- 수동 닫기(close) 및 페이지 이동(reset) 시 예약된 setTimeout을 명시적으로 클린업하여 사이드 이펙트를 방지합니다.
- 전역 ModalProvider 도입에 따라 ModalBasic 내부의 <Modal> 컨테이너 태그 제거 - Modal, ModalBasic을 사용하는 관련 컴포넌트 일괄 수정
📝 WalkthroughWalkthrough애플리케이션 전반의 모달 관리를 로컬 훅에서 중앙 집중식 Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component
participant ModalStore as modalStore
participant ModalProvider as ModalProvider
participant UI as Modal UI
Component->>ModalStore: open({id, content, autoPlay?, onClose?})
ModalStore->>ModalStore: add modal, 시작 타이머(선택적)
ModalStore->>ModalProvider: notify subscribers (modals)
ModalProvider->>UI: render modal items
Note over Component,UI: 사용자 상호작용
Component->>ModalStore: close(id)
ModalStore->>ModalStore: cancel timer, remove modal
ModalStore->>ModalStore: 실행 onClose 콜백(있으면)
ModalStore->>ModalProvider: notify subscribers (modals)
ModalProvider->>UI: 제거된 모달 반영
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 제안 라벨
제안 리뷰어
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 빌드 결과✅ 린트 검사 완료 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shared/ui/modal/modal-basic.tsx (1)
27-41:⚠️ Potential issue | 🟠 MajorX 버튼이
onClose액션을 타지 않습니다.지금 구조에서는
Modal.XButton이ModalProvider의onClose만 호출하고,ModalBasicProps.onClose는 취소 버튼에서만 실행됩니다. 그래서 취소 버튼과 X 버튼의 정리 로직이 달라질 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/modal/modal-basic.tsx` around lines 27 - 41, The X button currently invokes the provider's close handler instead of the modal instance's onClose prop, causing inconsistent cleanup; update ModalBasic (the JSX block using Modal.XButton, Modal.Buttons, and the props from ModalBasicProps.onClose) so that Modal.XButton receives and calls the same onClose handler passed into the component (e.g., forward the onClose prop down to Modal.XButton or attach a click handler that calls ModalBasicProps.onClose) ensuring both the X button and the cancel Button call the identical onClose logic.src/features/experience-matching/ui/select-company/select-company.tsx (1)
19-19: 🛠️ Refactor suggestion | 🟠 MajorProps 타입을 별도로 정의하는 것을 권장해요.
코딩 가이드라인에 따르면 Props 타입명은 '컴포넌트명Props' 형식을 사용해야 해요. 인라인 타입 대신
SelectCompanyProps인터페이스로 분리하면 가독성과 재사용성이 향상돼요.♻️ 수정 제안
+interface SelectCompanyProps { + onClick: () => void; +} + -export const SelectCompany = ({ onClick }: { onClick: () => void }) => { +export const SelectCompany = ({ onClick }: SelectCompanyProps) => {As per coding guidelines: "Props 타입명이 '컴포넌트명Props' 형식인지 확인"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/experience-matching/ui/select-company/select-company.tsx` at line 19, The SelectCompany component currently uses an inline prop type; extract that into a named interface SelectCompanyProps and update the component signature to use it (e.g., function SelectCompany(props: SelectCompanyProps) or const SelectCompany = ({ onClick }: SelectCompanyProps) => ...), defining SelectCompanyProps with onClick: () => void to follow the "ComponentNameProps" naming guideline and improve readability/reuse (refer to SelectCompany and onClick in the diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/experience-detail/model/use-leave-confirm.tsx`:
- Around line 49-53: The confirmLeave callback currently only calls
blocker.proceed(); update it to also explicitly close the leave modal by calling
modalStore.close(LEAVE_MODAL_ID) (same as cancelLeave) so the modal is dismissed
immediately and the behavior is consistent; locate the confirmLeave function in
use-leave-confirm.tsx and add modalStore.close(LEAVE_MODAL_ID) before or after
blocker.proceed() to ensure the modal is closed on confirmation.
- Line 28: Move the LEAVE_MODAL_ID constant out of the hook to module scope:
declare const LEAVE_MODAL_ID = "leave-confirm-modal" above the useLeaveConfirm
hook definition in use-leave-confirm.tsx (and export it if other modules need
it), then remove the in-hook declaration so all references inside the hook
(e.g., any usages within useLeaveConfirm) point to the module-level constant to
improve readability and reusability.
In `@src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx`:
- Around line 42-55: The delete modal currently calls modalStore.reset() in
handleOpenDeleteModal which closes all global modals; change it to open the
ModalBasic with a fixed unique id (e.g. const id = 'experience-delete') passed
to modalStore.open and then replace modalStore.reset() in both onClose and
onConfirm with modalStore.close(id) so only this modal is closed (keep onConfirm
calling onClickDelete() before modalStore.close(id)). Ensure you pass the id
when opening and use the same id in onClose/onConfirm.
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Line 66: The modal title hardcodes the Korean object particle ("을/를") and
should adapt to whether selectedCompany.name ends with a batchim; update the
text in Modal.Title where selectedCompany.name is used to either (a) call a
small utility (e.g., a josa helper like getJosa(name, '을', '를') or
hasBatchim-based function) and render "{selectedCompany.name}{josa} 선택하셨습니다", or
(b) switch to a neutral phrasing such as "선택한 기업: {selectedCompany.name}" to
avoid particle logic; modify the component to import/implement that josa helper
and use it around selectedCompany.name in the Modal.Title or replace the string
with the neutral form.
- Around line 60-77: The function handleSearch is misnamed because it doesn't
perform a search but confirms the selected company and shows a loading modal;
rename the function (e.g., to handleSelectConfirm or handleCompanySelect) and
update all references/usages to that new name (such as any onClick or prop
handlers that currently call handleSearch) so callers invoke the new identifier;
modify the function declaration for handleSearch to the chosen name and adjust
any exports/props passed into child components to match the new name, preserving
the existing logic that checks selectedCompany, opens modalStore, calls
setCompany(selectedCompany), and invokes onClick().
In `@src/shared/model/store/modal.store.ts`:
- Around line 57-59: The reset() method currently just clears _modalList and
calls _listner, which skips per-modal cleanup (timers and onClose); change
reset() to iterate over the existing _modalList (use a shallow copy) and call
the existing close(...) code-path for each modal (e.g., invoking the store's
close(id) or the modal's onClose/clearTimeout logic) so all timers and onClose
handlers run, then set _modalList = [] and invoke _listner once; ensure you
reference the existing close(...) function and _modalList/_listner symbols so
you reuse the proper cleanup logic rather than simply emptying the array.
- Around line 23-30: The modal ID generation in open() using new
Date().toString() can collide when open() is called rapidly; update open() to
generate collision-resistant IDs (e.g., use crypto.randomUUID() or a UUID v4
library, or a monotonic counter) instead of new Date().toString(); ensure the
generated id is assigned to new_modal.id and that existing close()/filter()
logic that relies on id (and this._modalList) continues to work with the new ID
format (update imports if you choose a UUID library).
- Around line 3-8: Change the ModalItem object shape from a type alias to an
interface: replace the `type ModalItem = { ... }` declaration with `interface
ModalItem { id: string; content: ReactNode; onClose?: () => void; autoPlay?:
number; }` so it follows the project's guideline of using interface for object
shapes; update any imports/exports or references to ModalItem in modal.store.ts
or other files if necessary to ensure the symbol name remains the same and
compiles.
---
Outside diff comments:
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Line 19: The SelectCompany component currently uses an inline prop type;
extract that into a named interface SelectCompanyProps and update the component
signature to use it (e.g., function SelectCompany(props: SelectCompanyProps) or
const SelectCompany = ({ onClick }: SelectCompanyProps) => ...), defining
SelectCompanyProps with onClick: () => void to follow the "ComponentNameProps"
naming guideline and improve readability/reuse (refer to SelectCompany and
onClick in the diff).
In `@src/shared/ui/modal/modal-basic.tsx`:
- Around line 27-41: The X button currently invokes the provider's close handler
instead of the modal instance's onClose prop, causing inconsistent cleanup;
update ModalBasic (the JSX block using Modal.XButton, Modal.Buttons, and the
props from ModalBasicProps.onClose) so that Modal.XButton receives and calls the
same onClose handler passed into the component (e.g., forward the onClose prop
down to Modal.XButton or attach a click handler that calls
ModalBasicProps.onClose) ensuring both the X button and the cancel Button call
the identical onClose logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 071436a1-c2fd-469b-a589-ac7dab18f1f0
📒 Files selected for processing (10)
src/app/providers/modal-provider.tsxsrc/app/routes/root-layout.tsxsrc/features/experience-detail/model/use-leave-confirm.tsxsrc/features/experience-detail/ui/experience-viewer/experience-viewer.tsxsrc/features/experience-matching/ui/select-company/select-company.tsxsrc/pages/experience-detail/experience-detail-page.tsxsrc/shared/model/store/index.tssrc/shared/model/store/modal.store.tssrc/shared/ui/modal/modal-basic.tsxsrc/shared/ui/textfield/textfield.tsx
| const handleOpenDeleteModal = () => { | ||
| modalStore.open( | ||
| <ModalBasic | ||
| title="이 경험을 삭제하시겠습니까?" | ||
| subTitle="작성한 내용은 즉시 제거되며, 복구할 수 없습니다." | ||
| closeText="취소" | ||
| confirmText="삭제" | ||
| onClose={() => modalStore.reset()} // 취소 시 닫기 | ||
| onConfirm={() => { | ||
| onClickDelete(); // 실제 삭제 동작 | ||
| modalStore.reset(); // 모달 닫기 | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
이 확인창에서 reset()을 쓰면 다른 전역 모달까지 같이 닫힙니다.
onClose와 onConfirm이 모두 modalStore.reset()을 호출해서, 이 삭제 모달과 무관한 다른 모달도 함께 사라집니다. 여기서는 고정 ID를 넘기고 modalStore.close(id)로 자기 자신만 닫도록 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-detail/ui/experience-viewer/experience-viewer.tsx`
around lines 42 - 55, The delete modal currently calls modalStore.reset() in
handleOpenDeleteModal which closes all global modals; change it to open the
ModalBasic with a fixed unique id (e.g. const id = 'experience-delete') passed
to modalStore.open and then replace modalStore.reset() in both onClose and
onConfirm with modalStore.close(id) so only this modal is closed (keep onConfirm
calling onClickDelete() before modalStore.close(id)). Ensure you pass the id
when opening and use the same id in onClose/onConfirm.
src/features/experience-matching/ui/select-company/select-company.tsx
Outdated
Show resolved
Hide resolved
| open( | ||
| content: ReactNode, | ||
| autoPlay?: number, | ||
| onClose?: () => void, | ||
| id: string = new Date().toString() | ||
| ) { | ||
| const new_modal = { id: id, content: content, autoPlay, onClose }; | ||
| this._modalList = [...this._modalList, new_modal]; |
There was a problem hiding this comment.
기본 모달 ID가 충돌할 수 있습니다.
new Date().toString()는 충돌 없는 식별자가 아니어서 짧은 시간 안에 여러 open()이 호출되면 같은 ID가 나올 수 있습니다. 그러면 close()/타이머 정리가 엉키고, filter()가 여러 모달을 한 번에 지울 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/model/store/modal.store.ts` around lines 23 - 30, The modal ID
generation in open() using new Date().toString() can collide when open() is
called rapidly; update open() to generate collision-resistant IDs (e.g., use
crypto.randomUUID() or a UUID v4 library, or a monotonic counter) instead of new
Date().toString(); ensure the generated id is assigned to new_modal.id and that
existing close()/filter() logic that relies on id (and this._modalList)
continues to work with the new ID format (update imports if you choose a UUID
library).
- 동적변수명 (한글/영어) 뒤에 오는 조사를 구분하여 붙이기 위한 es-hangul 패키지 설치
- 상수(LEAVE_MODAL_ID) 훅 외부로 이동 - confirmLeave 모달에서 명시적으로 모달이 닫히도록 modal.close() 호출 - modalItem interface 변경 - reset 메서드에서 예약된 타이머도 함께 제거
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/shared/model/store/modal.store.ts (2)
57-63:⚠️ Potential issue | 🟠 Major
reset()이close()경로를 우회해서 정리가 달라집니다.여기서는 타이머와 리스트만 비우고 각 모달의
onClose는 호출하지 않습니다. 그런데src/app/providers/modal-provider.tsx:23-25에서 pathname 변경마다modalStore.reset()을 호출하므로, 라우트 이동은 사실상 “모달 닫힘”의 한 경로인데 cleanup semantics가 달라집니다. 기존 모달들을 순회하면서close(id)를 재사용하도록 맞추는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 57 - 63, reset() currently clears timers and the _modalList without invoking each modal's onClose, which diverges from the close() cleanup path used on navigation; change reset() to iterate the current _modalList (shallow copy it first) and call close(id) for each modal id so the existing close(id) semantics (onClose callbacks, timer cleanup, and listener notifications) are reused; ensure you still clear _timers/_modalList as needed after using close(id) to avoid double-cleanup or keeping stale references, and reference the reset(), close(id), _modalList, _timers, and _listner symbols when making the change.
23-30:⚠️ Potential issue | 🟠 Major기본 모달 ID 생성이 충돌 가능합니다.
new Date().toString()는 짧은 시간 안에 여러open()이 호출되면 같은 값을 만들 수 있어서, 타이머가 덮어써지거나close(id)가 의도치 않게 여러 모달에 영향을 줄 수 있습니다. 충돌 없는 ID 생성 방식으로 바꿔두는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 23 - 30, The modal ID generation in open(...) (new_modal) uses new Date().toString(), which can collide when open() is called rapidly; replace that default with a collision-resistant generator (e.g., crypto.randomUUID() or a project-wide nanoid utility or an internal monotonic counter) so each created modal gets a unique id; update the open function's default id behavior (and any tests/consumers if they rely on stringified Date) to use the chosen generator and keep references to the id field (new_modal.id, close(id)) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/experience-matching/ui/select-company/select-company.tsx`:
- Around line 60-79: handleSearch can be re-entered on rapid clicks causing
setCompany(selectedCompany) and onClick() to run multiple times; add a pending
guard (e.g., a useRef or state boolean like isPending) checked at the top of
handleSearch to return early if true, set isPending = true immediately before
calling modalStore.open, and clear/reset isPending in the modalStore.open
completion callback (the function that currently calls setCompany and onClick)
or after the 3s timeout so the flow only executes once per selection;
alternatively disable the related button when isPending is true. Ensure you
reference and update the same pending flag around handleSearch, modalStore.open,
setCompany and onClick to prevent double execution.
In `@src/shared/model/store/modal.store.ts`:
- Around line 12-20: The current implementation stores a single callback in
_listner and clears it globally in unsubscribe(), which overwrites earlier
subscribers and allows one component's cleanup to remove others; change _listner
to a collection (e.g., Set or array) of callbacks, update subscribe(callback:
(list: ModalItem[]) => void) to add the callback to that collection and return a
dedicated cleanup function that removes only that callback, and update
unsubscribe() (or remove it) so it no longer nulls all listeners; update any
emit/notify logic to iterate the collection when broadcasting modal list
changes.
---
Duplicate comments:
In `@src/shared/model/store/modal.store.ts`:
- Around line 57-63: reset() currently clears timers and the _modalList without
invoking each modal's onClose, which diverges from the close() cleanup path used
on navigation; change reset() to iterate the current _modalList (shallow copy it
first) and call close(id) for each modal id so the existing close(id) semantics
(onClose callbacks, timer cleanup, and listener notifications) are reused;
ensure you still clear _timers/_modalList as needed after using close(id) to
avoid double-cleanup or keeping stale references, and reference the reset(),
close(id), _modalList, _timers, and _listner symbols when making the change.
- Around line 23-30: The modal ID generation in open(...) (new_modal) uses new
Date().toString(), which can collide when open() is called rapidly; replace that
default with a collision-resistant generator (e.g., crypto.randomUUID() or a
project-wide nanoid utility or an internal monotonic counter) so each created
modal gets a unique id; update the open function's default id behavior (and any
tests/consumers if they rely on stringified Date) to use the chosen generator
and keep references to the id field (new_modal.id, close(id)) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aed1f2f9-d101-4c3d-9841-17fa96a619e0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yamland included by none
📒 Files selected for processing (4)
package.jsonsrc/features/experience-detail/model/use-leave-confirm.tsxsrc/features/experience-matching/ui/select-company/select-company.tsxsrc/shared/model/store/modal.store.ts
| const handleSearch = () => { | ||
| if (!selectedCompany) return; | ||
| // 기업 선택 후, 대기하는 모달 | ||
| modalStore.open( | ||
| <> | ||
| <Modal.Content type="auto"> | ||
| <Modal.Title> | ||
| {josa(selectedCompany.name, "을/를")} 선택하셨습니다 | ||
| </Modal.Title> | ||
| <Modal.SubTitle>기업분석 내용을 불러오는 중입니다.</Modal.SubTitle> | ||
| </Modal.Content> | ||
| <Modal.Image /> | ||
| </>, | ||
| 3000, | ||
| () => { | ||
| setCompany(selectedCompany); | ||
| onClick(); | ||
| } | ||
| } | ||
| prevIsOpen.current = isOpen; | ||
| }, [isOpen, selectedCompany, onClick, setCompany]); | ||
| ); | ||
| }; |
There was a problem hiding this comment.
이 플로우는 재진입 방지가 필요합니다.
handleSearch가 클릭마다 새 3초 타이머를 등록하는데, 선택 버튼은 onSearch?.()를 동기적으로 바로 호출합니다. 그래서 빠른 더블클릭이면 setCompany(selectedCompany)와 onClick()이 여러 번 실행될 수 있습니다. 이 구간은 pending 플래그로 한 번만 실행되게 잠그거나, 버튼을 즉시 비활성화하는 방어가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/experience-matching/ui/select-company/select-company.tsx` around
lines 60 - 79, handleSearch can be re-entered on rapid clicks causing
setCompany(selectedCompany) and onClick() to run multiple times; add a pending
guard (e.g., a useRef or state boolean like isPending) checked at the top of
handleSearch to return early if true, set isPending = true immediately before
calling modalStore.open, and clear/reset isPending in the modalStore.open
completion callback (the function that currently calls setCompany and onClick)
or after the 3s timeout so the flow only executes once per selection;
alternatively disable the related button when isPending is true. Ensure you
reference and update the same pending flag around handleSearch, modalStore.open,
setCompany and onClick to prevent double execution.
- 기존 단일 listener 구조에서 다중 subscriber를 지원하도록 observer 패턴 구조 수정 - subscribe가 cleanup 함수를 반환하도록 변경하여 각 subscriber가 독립적으로 해제되도록 개선 - store 레이어의 확장성과 안정성을 고려한 observer 패턴 구조 명확화
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/shared/model/store/modal.store.ts (2)
63-69:⚠️ Potential issue | 🟠 Major
reset()이 모달별 cleanup 경로를 우회합니다.지금 구현은 타이머만 지우고 리스트를 비워서 각 모달의
onClose가 호출되지 않습니다. 라우트 이동 시 cleanup을onClose에 의존하는 호출부는 정리가 누락되므로, 현재 목록의 사본을 순회하면서 기존close(id)경로를 재사용하는 쪽이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 63 - 69, The reset() method currently clears timers and empties _modalList directly, bypassing each modal's onClose; instead, create a shallow copy of _modalList and iterate it, calling the existing close(id) path for each modal (so onClose runs), then clear this._timers and finally notify(); reference the reset(), _timers, _modalList and close(id) symbols and ensure you reuse close(id) rather than directly mutating _modalList.
29-45:⚠️ Potential issue | 🟠 Major기본 모달 ID는 충돌할 수 있습니다.
new Date().toString()는 짧은 시간 안에 여러open()이 호출되면 같은 값을 만들 수 있어서,close()/타이머 정리가 다른 모달까지 엮일 수 있습니다. 기본값은crypto.randomUUID()처럼 충돌 저항성이 있는 방식으로 바꾸는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/model/store/modal.store.ts` around lines 29 - 45, The default modal ID generation in open(...) using new Date().toString() can collide; update the open method signature (open in modal.store.ts) so the default id is generated with a collision-resistant approach (e.g., crypto.randomUUID()) instead of new Date().toString(); ensure the rest of the logic that references id (new_modal creation, this._modalList update, this._timers.set(id, ...), and close(id) usage) continues to work with the new ID format and keep the existing behavior when a caller supplies an explicit id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/providers/modal-provider.tsx`:
- Around line 16-21: The current modal subscription uses useState + useEffect
(modalStore.subscribe -> setModals) which is not safe for concurrent rendering;
replace this pattern by implementing a getSnapshot function that reads current
modalStore state and subscribe function that delegates to modalStore.subscribe,
then call React's useSyncExternalStore(subscribe, getSnapshot) instead of
useState/useEffect so the component reads synchronized snapshots from
modalStore; update references to useSyncExternalStore, modalStore, and remove
the useEffect/setModals logic accordingly.
- Line 6: The file imports Modal using a deep relative path
("../../shared/ui/modal/modal") which breaks consistency with the other alias
imports; replace that import with the project's alias import (e.g.,
"@/shared/ui/modal/modal") in the modal-provider file where Modal is imported so
it matches the existing alias style, and ensure the import statement referencing
Modal is updated accordingly (no other changes required).
- Around line 8-12: The local ModalItem interface in modal-provider.tsx
duplicates the store type; remove the local declaration and import the canonical
ModalItem type exported by the store (the ModalItem exported from modal.store)
so both the provider and store share a single definition; update any uses in
ModalProvider (e.g., state, props, functions referencing ModalItem) to use the
imported type and ensure the store file exports ModalItem if it doesn't already.
In `@src/shared/model/store/modal.store.ts`:
- Around line 35-36: Rename the local variable new_modal to camelCase newModal
and build the modal object using property shorthand (e.g., { id, content,
autoPlay, onClose }) instead of the verbose form, then append it to
this._modalList (update any occurrences of new_modal to newModal); this keeps
consistency with _modalList and other identifiers in this file.
- Line 13: The private field _timers is typed as Map<string, NodeJS.Timeout>,
which can break in browser builds; change its type to Map<string,
ReturnType<typeof setTimeout>> (and adjust the initializer if needed) so the
timeout type is environment-agnostic; also update any code that assumes
NodeJS.Timeout (e.g., clearTimeout calls or variable annotations) to use the new
ReturnType<typeof setTimeout> type or infer the type from setTimeout results.
---
Duplicate comments:
In `@src/shared/model/store/modal.store.ts`:
- Around line 63-69: The reset() method currently clears timers and empties
_modalList directly, bypassing each modal's onClose; instead, create a shallow
copy of _modalList and iterate it, calling the existing close(id) path for each
modal (so onClose runs), then clear this._timers and finally notify(); reference
the reset(), _timers, _modalList and close(id) symbols and ensure you reuse
close(id) rather than directly mutating _modalList.
- Around line 29-45: The default modal ID generation in open(...) using new
Date().toString() can collide; update the open method signature (open in
modal.store.ts) so the default id is generated with a collision-resistant
approach (e.g., crypto.randomUUID()) instead of new Date().toString(); ensure
the rest of the logic that references id (new_modal creation, this._modalList
update, this._timers.set(id, ...), and close(id) usage) continues to work with
the new ID format and keep the existing behavior when a caller supplies an
explicit id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34e98715-258b-4518-ab8d-0c8d7f82eca8
📒 Files selected for processing (2)
src/app/providers/modal-provider.tsxsrc/shared/model/store/modal.store.ts
|
|
||
| import { modalStore } from "@/shared/model/store"; | ||
|
|
||
| import { Modal } from "../../shared/ui/modal/modal"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
깊은 상대 경로 import는 alias로 통일해주세요.
이 파일은 Line 4에서 이미 alias import를 쓰고 있는데, ../../shared/ui/modal/modal만 상대 경로라 이동/리팩터링에 더 취약합니다. @/shared/ui/modal/modal로 맞추는 편이 일관성과 유지보수성에 좋습니다. As per coding guidelines, "@shared, @features, @widgets 등 alias 기반 절대 경로 import 사용 권장" and "깊은 상대 경로(../../..) 사용은 가독성과 유지보수 측면에서 지양".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/providers/modal-provider.tsx` at line 6, The file imports Modal using
a deep relative path ("../../shared/ui/modal/modal") which breaks consistency
with the other alias imports; replace that import with the project's alias
import (e.g., "@/shared/ui/modal/modal") in the modal-provider file where Modal
is imported so it matches the existing alias style, and ensure the import
statement referencing Modal is updated accordingly (no other changes required).
| interface ModalItem { | ||
| id: string; | ||
| content: ReactNode; | ||
| autoPlay?: number; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
store의 ModalItem 타입을 재사용하는 편이 안전합니다.
여기 타입이 src/shared/model/store/modal.store.ts:3-8와 거의 동일해서, 필드가 추가되거나 바뀌면 provider 쪽만 조용히 drift할 수 있습니다. store에서 타입을 export하거나 별도 shared type으로 분리해 한 군데서 관리하는 편이 좋겠습니다. As per coding guidelines, "중복 코드 제거 및 재사용 가능한 구조 제안".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/providers/modal-provider.tsx` around lines 8 - 12, The local
ModalItem interface in modal-provider.tsx duplicates the store type; remove the
local declaration and import the canonical ModalItem type exported by the store
(the ModalItem exported from modal.store) so both the provider and store share a
single definition; update any uses in ModalProvider (e.g., state, props,
functions referencing ModalItem) to use the imported type and ensure the store
file exports ModalItem if it doesn't already.
| const [modals, setModals] = useState<ModalItem[]>([]); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribe = modalStore.subscribe(setModals); | ||
| return unsubscribe; | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
외부 store 구독은 useSyncExternalStore로 바꾸는 편이 더 견고합니다.
지금 패턴은 useState([])로 시작한 뒤 effect에서 subscribe만 등록하므로, 현재 snapshot 동기화와 concurrent rendering 안정성을 React가 직접 보장해주지 않습니다. modalStore가 외부 store라면 getSnapshot을 제공하고 useSyncExternalStore로 연결하는 쪽이 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/providers/modal-provider.tsx` around lines 16 - 21, The current modal
subscription uses useState + useEffect (modalStore.subscribe -> setModals) which
is not safe for concurrent rendering; replace this pattern by implementing a
getSnapshot function that reads current modalStore state and subscribe function
that delegates to modalStore.subscribe, then call React's
useSyncExternalStore(subscribe, getSnapshot) instead of useState/useEffect so
the component reads synchronized snapshots from modalStore; update references to
useSyncExternalStore, modalStore, and remove the useEffect/setModals logic
accordingly.
| class ModalStore { | ||
| private _modalList: ModalItem[] = []; // 모달 리스트 관리 | ||
| private _listeners = new Set<(list: ModalItem[]) => void>(); | ||
| private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
브라우저 코드에서 NodeJS.Timeout 고정은 타입 환경에 따라 깨질 수 있습니다.
클라이언트 번들에서는 setTimeout 반환형이 DOM/lib 설정에 따라 달라서, 여기처럼 NodeJS.Timeout로 고정하면 타입 충돌이 날 수 있습니다. ReturnType<typeof setTimeout>로 두면 런타임 환경에 독립적으로 안전합니다.
🔧 제안 코드
- private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리
+ private _timers = new Map<string, ReturnType<typeof setTimeout>>(); // 타이머 관리📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _timers = new Map<string, NodeJS.Timeout>(); // 타이머 관리 | |
| private _timers = new Map<string, ReturnType<typeof setTimeout>>(); // 타이머 관리 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/model/store/modal.store.ts` at line 13, The private field _timers
is typed as Map<string, NodeJS.Timeout>, which can break in browser builds;
change its type to Map<string, ReturnType<typeof setTimeout>> (and adjust the
initializer if needed) so the timeout type is environment-agnostic; also update
any code that assumes NodeJS.Timeout (e.g., clearTimeout calls or variable
annotations) to use the new ReturnType<typeof setTimeout> type or infer the type
from setTimeout results.
| const new_modal = { id: id, content: content, autoPlay, onClose }; | ||
| this._modalList = [...this._modalList, new_modal]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
변수명은 camelCase로 맞춰주세요.
new_modal은 이 파일의 다른 식별자와 컨벤션이 달라서 newModal이 더 일관적입니다. 객체 리터럴도 shorthand로 함께 정리할 수 있습니다. As per coding guidelines, "Use camelCase for variable names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/model/store/modal.store.ts` around lines 35 - 36, Rename the local
variable new_modal to camelCase newModal and build the modal object using
property shorthand (e.g., { id, content, autoPlay, onClose }) instead of the
verbose form, then append it to this._modalList (update any occurrences of
new_modal to newModal); this keeps consistency with _modalList and other
identifiers in this file.
✏️ Summary
📑 Tasks
기존의 구조는 비슷하지만 각각 다른 UI의 모달 컴포넌트을 쉽게 구성할 수 있도록 컴파운트 패턴을 활용하여 UI의 유연성을 높였습니다. 또한 모달이
open,close상태를 기준으로 열리고 닫힐 수 있도록use-modal훅을 분리하여 상태를 관리하도록 했습니다.하지만 실제 모달 컴포넌트를 코드에 사용하는 측면에 있어 두 가지 문제점이 있다고 생각했습니다.
만약 어떤 컴포넌트(
select-company.tsx)에서 사용하는 모달의 개수가 2가지 이상이 된다면, 각각의 모달의 상태를 위해각각
use-modal훅을 호출하여 각자의isOpen를 관리하도록 해야 했습니다. 한 가지 모달만 사용할 때도 무조건use-modal훅을 호출해야 한다는 사실도 DX 관점에서 좋지 않다는 생각이 들기도 했습니다.또한 JSX 코드에서 실제 컴포넌트 내용과는 약간 무관한, 동떨어져 있는 모달 코드를 JSX 코드에 함께 작성해야해 컴포넌트의 의미를 명확하게 하지 못하는 한계가 있다고 느껴졌습니다.
이러한 문제점을 해결하기 위해
observer pattern을 도입하기로 했습니다.observer pattern을 도입한 이유는 다음과 같습니다.ModalContainer한 곳으로 위임하여, 개별 컴포넌트의 JSX 구조를 본연의 역할에 맞게 간결하게 유지하고 관심사를 분리하는 것입니다.modalStore.open()과 같은 단순한 명령형 함수 호출만으로 쉽게 모달을 제어할 수 있어 개발 생산성을 높일 수 있습니다.observer class 구현 (
modal.store.ts)modalList 배열을 통해 모달 상태를 관리하는 순수 JS 클래스를 선언하고, 싱글톤 인스턴스로 export 하였습니다.
[ + 추가 수정사항 ]
현재 구현에서는
ModalProvider하나만subscribe하고 있기 때문에 실제 동작에는 문제가 없습니다. 다만ModalStore의subscribe()가 단일 콜백만 보관하는 구조라, 이후 다른 컴포넌트에서 동일한 store를 구독하게 될 경우 기존 subscriber가 덮어써지거나unsubscribe()호출 시 의도하지 않게 다른 subscriber까지 해제될 가능성이 있습니다.Publisher–Subscriber 관계를 1:1이 아닌 1:N 구조로 명확하게 관리하는 것이 더 안전하기 때문에 이를 위해 내부 listener를 하나의 콜백이 아닌 컬렉션으로 관리하고,
subscribe()가 해당 subscriber만 해제할 수 있는 cleanup 함수를 반환하는 형태로 수정하였습니다.모달을 보여줄 Provider 정의 (
modal-provider.ts)modalStore를 실질적으로 구독하여 전역 상태가 변경될 때마다 _modalList에 등록된 모달들을 렌더링하는 최상단 렌더링 컨테이너입니다. 특정 컴포넌트에서
modalStore.open()명령을 보내면, 상태가 변경되었음으로 판단하고 해당 모달을 보여주기 위해 렌더링되는 것입니다.컴포넌트는 오직 모달을 열어라라는 명령(
modalStore.open)만 내리기 때문에, 실제 UI 렌더링 책임은 전역 컨테이너로 넘어갔으며, 컴포넌트 내 JSX는 자신의 컴포넌트 내용 자체에만 집중할 수 있습니다.[추가적인 트러블 슈팅]
전역으로 모달을 분리하면서, 모달이 열린 상태에서 뒤로 가기를 누르거나 페이지를 이동할 경우 모달 자체의 상태가 변화한 것이 아니기 때문에
onClose이벤트가 발생하지 않아 이동 페이지에서도 모달이 계속 떠 있는 문제가 있었습니다.이를 위해
useLocation을 통해 pathname의 변경을 감지하고, 라우트가 바뀔 때modalStore.reset()을 호출하여 모달이 닫힐 수있도록 ModalProvider에 로직을 추가했습니다.👀 To Reviewer
use-modal은 이제 사용하지 않아 머지하기 전에 삭제할 예정입니다.use-modal이 사용하지 않게 됨에 따라경험 등록/보기페이지에서 사용되었던 훅을 제거하고modalStore의 메서드(open, reset)를 호출하여 사하도록 수정하였습니다.modal-provider.tsx에서 전담합니다. 따라서 주로 사용되던 모달 템플릿인modal-basic컴포넌트 내부에 선언되어 있던 래퍼 태그는 제거하였습니다.📸 Screenshot
🔔 ETC
es-hangul패키지를 설치하였습니다Summary by CodeRabbit